Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use token-based pagination for Jobs List operations #2810

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Use token-based pagination for Jobs List operations #2810

merged 1 commit into from
Oct 23, 2023

Conversation

alexott
Copy link
Contributor

@alexott alexott commented Oct 17, 2023 •
edited
Loading

Changes

As we couldn't easily switch databricks_job resource to use Go SDK, we were still using offset-based pagination that has a significant load onto the control plane. This PR changes list operation to use recommended token-based approach until we switch to Go SDK.

This fixes #2807

Tests

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • relevant acceptance tests are passing
  • using Go SDK

@alexott
Use token-based pagination for Jobs List operations
849068c 
As we couldn't easily switch `databricks_job` resource to use Go SDK, we were still using
offset-based pagination that has a significant load onto the control plane.  This PR
changes list operation to use recommended token-based approach until we switch to Go SDK.

This fixes #2807
@alexott alexott requested review from a team as code owners October 17, 2023 10:31
@alexott alexott requested review from mgyucht and removed request for a team October 17, 2023 10:31
oleksiidav
oleksiidav approved these changes Oct 17, 2023
View reviewed changes
Copy link

@oleksiidav oleksiidav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor nits

mgyucht
mgyucht reviewed Oct 19, 2023
View reviewed changes
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation seems good to me. Can we extend TestJobsData to make a second request with the page token?

jobs/resource_job.go
Jobs []Job `json:"jobs"`
HasMore bool `json:"has_more,omitempty"`
NextPageToken string `json:"next_page_token,omitempty"`
PrevPageToken string `json:"prev_page_token,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, you're adding PrevPageToken for completeness, even though we don't use it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only for completeness...

mgyucht
mgyucht approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Missed the addition of the next_page_token

@mgyucht mgyucht added this pull request to the merge queue Oct 23, 2023
Merged via the queue into master with commit 205b67c Oct 23, 2023
7 checks passed
@mgyucht mgyucht deleted the jobs-list-page-token branch October 23, 2023 15:17
@mgyucht mgyucht mentioned this pull request Oct 25, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleksiidav oleksiidav oleksiidav approved these changes

@mgyucht mgyucht mgyucht approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ISSUE] Exporter: Issue with databricks_jobs resource import
3 participants
@alexott @mgyucht @oleksiidav